Skip to content

fix: Narrow conditions for load_value to give invalid address - #6157

Open
amjames wants to merge 11 commits into
pybind:masterfrom
amjames:bugfix-6153
Open

fix: Narrow conditions for load_value to give invalid address#6157
amjames wants to merge 11 commits into
pybind:masterfrom
amjames:bugfix-6153

Conversation

@amjames

@amjames amjames commented Aug 28, 2026

Copy link
Copy Markdown

Description

This PR addresses #6153 when every extension that may load or construct an affected instance is built with the updated headers. It intentionally retains PYBIND11_INTERNALS_VERSION 12; older inline caster code can therefore still take the pre-existing unsafe lazy-allocation path. The updated side recovers storage collisions that it observes, but cannot prevent undefined behavior already triggered in stale code or revoke escaped pointers. See the mixed-v12 investigation, the subsequent ABI discussion, and the collision-recovery notes for the nuances.

Calling cls.__new__(cls) for a pybind11-bound class creates the Python object and its value/holder slots without constructing the C++ value. Previously, loading that object from bound code could lazily allocate raw storage and treat it as a live C++ object. Reading a member could therefore return uninitialized data, and virtual dispatch could load an invalid vtable pointer and segfault. A malformed pickle can reach the same state; PyTorch has a downstream mitigation in pytorch/pytorch#194647.

The complication is that pybind11's deprecated old-style placement-new __init__ and __setstate__ callbacks legitimately need access to uninitialized storage. This change preserves those callbacks without making that storage available to arbitrary bound-code loads.

Design

Construction is now tracked for each exact value_and_holder slot rather than for the whole Python instance. During an old-style constructor candidate, raw storage is reserved privately by the current loader frame. A one-shot authorization lets only that candidate's self conversion load that exact slot, either as argument zero or through the typed cast performed inside a legacy py::object callback. The pointer is not published in the instance until the native callback returns successfully.

All other loads while the slot is under construction raise ValueError, including:

  • re-entry while converting a later constructor argument;
  • new-style candidates in mixed old-/new-style overload chains;
  • nested initialization of the same value;
  • access through another base in Python multiple inheritance; and
  • concurrent access on free-threaded Python.

Failed candidates clean up their private storage before overload resolution continues. Successful construction publishes and finalizes the value before return-value conversion and post-call policies run. Pure new-style constructors use the same construction-state guard, but never receive the old-style storage authorization.

If stale v12 inline caster code publishes competing storage while an updated old-style constructor holds a private reservation, the updated code attempts to roll back the value-slot, registration, and holder state when it regains control. Cleanup uses the deallocator selected by the DSO that registered the type. Failures encountered during loader-frame destructor cleanup are reported with PyErr_WriteUnraisable rather than escaping from that destructor. This is recovery of pybind11's internal state, not a general rollback of arbitrary C++ side effects or escaped pointers.

Construction-state transitions and loads are protected by the instance critical section on free-threaded Python. The construction flag reuses available bits in the existing simple-instance bitfield and nonsimple status byte; PYBIND11_INTERNALS_VERSION remains 12.

Compatibility and tests

The regression coverage includes direct __new__ for bound classes and Python subclasses, ordinary pickle and manual __setstate__, failed old-style initialization and retry, later-argument re-entry, mixed old-/new-style overloads, nested initialization, Python multiple inheritance, and synchronized concurrent access. It also covers legacy callbacks whose self parameter is typed as either the bound C++ class or py::object.

Mixed-v12 collision tests freeze the relevant lazy-publication behavior from the pre-PR caster in a separate extension module. They cover failure before the constructor callback completes, collision after successful placement construction, retryability, the default holder, and py::smart_holder.

Suggested changelog entry

Prevent bound code built with updated headers from treating a pybind11 instance whose C++ value was never constructed, for example after direct __new__, as a live C++ object. Preserve deprecated old-style placement-new constructors and pickle __setstate__ callbacks while rejecting reentrant, nested, cross-base, and concurrent loads by updated casters until C++ construction completes.

AI assistance

The original changes were authored with assistance from Claude. The follow-up redesign and regression coverage were developed with Codex GPT-5.6-sol ultra and independently audited by a separate agent.


📚 Documentation preview 📚: https://pybind11--6157.org.readthedocs.build/

…ting python object

fixes: pybind#6153

Objects initialized with `cls.__new__(cls)` (`cls` is a pybind11 bound
type). Will not have the C++ object allocated. When hitting `load_value`
storage is allocated but not initialized, calling a virtual method will
load a garbage vptr and segfault. This is similar to pybind#2152, but the
guard in metaclass `__call__` is not triggered when using `__new__`.

Protect against giving a pointer to garbage in all cases except the
`__init__` + `__setstate__` path.

Authored with claude
…y allocation for old-style constructors

If an old-style placement-new `__init__`/`__setstate__` failed after
`self` was loaded, the lazily allocated storage stayed behind with a
null-holder instance, so the uninitialized-value guard never fired again
and later use read uninitialized memory. `instance_construction_scope`
now tracks the constructor's `value_and_holder` and frees storage that
was lazily allocated during a construction that did not complete.

Also arm the scope only when the overload chain contains an old-style
constructor. New-style constructors receive `self` directly and never
need lazy allocation, so reentrant loads of the half-built instance now
raise `ValueError` instead of handing out uninitialized storage.

Assisted-by: ClaudeCode:claude-fable-5
Claude-Session: https://claude.ai/code/session_01TQXCSykMn5EL7sc6VgTUTC
@henryiii

Copy link
Copy Markdown
Collaborator

I pushed two fixes from Fable. Both started with failing tests, then fixed.

🤖 AI text below 🤖

Review complete: 12 candidates checked, 2 survived (both confirmed), 10 refuted.

1. Incomplete fix — failed old-style __init__ reopens the segfault (type_caster_base.h:1173, empirically reproduced, exit 139). If an old-style placement-new __init__ throws (or a later argument fails to convert) after self was loaded, load_value has already lazily allocated vptr but placement-new never ran. Nothing deallocates it on the error path, so the next method call sees vptr != nullptr, skips the new ValueError, and hands out uninitialized memory. The new construction_in_progress bit is exactly the hook to close this: on constructor-dispatch failure, free a value that was lazily allocated while the holder was never constructed.

2. Guard armed too broadly (pybind11.h:1008). The scope arms for every constructor chain, but new-style py::init never needs lazy allocation (self is injected directly), so reentrant access to a half-built instance during modern construction still gets silent garbage instead of the new diagnostic. Cheap narrowing: arm only when the chain has an old-style constructor (is_constructor && !is_new_style_constructor). Not a regression, but a missed improvement.

Notable refutations: cross-module ABI safe (layout unchanged, bit zero-filled by tp_alloc); the bitfield RMW race matches the accepted pre-existing pattern (has_patients, owned, etc.); throwing instead of overload fallthrough replaces UB, not working behavior; all three cleanup nits stand as written.

@espressolee espressolee left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I independently reviewed exact head 66f8f3760f02cb596b28e552fc4f95cd79586b7a against base 5e9611aacc0bdd2054aa36800055014ebcd8e805 on macOS/arm64 with CPython 3.14.6 and 3.14.6t.

The submitted checks work for their covered paths: direct use of a __new__-only instance raises, failed old-style initialization is cleaned up, new-style constructor re-entry raises, pickle round trips survive, and test_class.py is 43/43 in regular Debug, regular NDEBUG, and free-threaded Debug builds.

There is still a blocking re-entry hole in the old-style placement-new path. instance_construction_scope marks the instance for the entire constructor dispatcher, and loading the old-style self argument lazily allocates vptr. If conversion of a later constructor argument executes Python and calls another bound method on the same object, that method sees the now-non-null raw pointer and bypasses the new null-value guard, even though placement-new has not run. A virtual call then reads the uninitialized vtable.

Minimal shape using the PR's own OldStyleInit fixture:

obj = m.OldStyleInit.__new__(m.OldStyleInit)

class Reenter:
    def __index__(self):
        obj.v_data()  # dispatches through unconstructed storage
        raise TypeError

obj.__init__(Reenter())

Exact-head results, fresh processes:

  • CPython 3.14.6 Debug: 5/5 SIGSEGV
  • CPython 3.14.6 NDEBUG: 5/5 SIGSEGV
  • CPython 3.14.6t Debug with the GIL disabled: 5/5 SIGSEGV
  • standalone exact-base build: 5/5 SIGSEGV

This is not a regression introduced by the PR, but it remains inside the same claimed invariant: old-style construction should be the only permitted lazy-allocation path without making arbitrary re-entrant native access safe. The current instance-wide flag cannot distinguish the constructor's own self load from a method call (or another thread) while construction is in progress.

Please scope the permission to the old-style constructor's own self conversion, and make every other load before successful construction raise. A regression where conversion of a later argument re-enters the same instance would cover the gap. I am not prescribing a particular implementation because preserving multi-overload old-style dispatch and free-threaded access requires the permission to be call/load-specific rather than merely an instance-wide time window.

Limits: I built the changed test_class module rather than the full local matrix; the exact-head remote matrix is green. The finding is deterministic in all three locally tested configurations above.

@rwgk

rwgk commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

I'll take a stab at this using codex gpt-5.6-sol ultra.

My starting point:


Verdict

PR 6157 should not merge at its current head, 66f8f37. The post-Henry review found a genuine blocking hole, and I independently reproduced it on your locally merged branch: the process terminates with SIGSEGV.

What the PR gets right

The original problem is real and security-relevant: direct __new__ creates the Python shell without constructing its C++ value, after which load_value() used to hand out raw, unconstructed storage. This affects PyTorch and malformed pickle handling; PyTorch has already merged a downstream mitigation. Issue #6153, PyTorch mitigation.

The current PR correctly handles several important paths:

  • Ordinary access to a __new__-only object raises instead of reading garbage.
  • Normal pickle round trips continue working.
  • Henry’s cleanup correctly frees lazily allocated storage after a failed old-style constructor.
  • Henry correctly prevents pure new-style constructors from enabling lazy allocation.
  • I see no conventional ABI-size problem: the added bit still fits in the existing bitfield allocation.

All four submitted focused tests pass, and the current GitHub matrix has 77 successful checks and two expected skips.

Blocking finding

The permission is scoped to the entire instance and entire constructor dispatcher, rather than to the old-style constructor’s own self conversion.

The sequence is:

  1. The dispatcher marks the instance as under construction in pybind11.h.
  2. Converting the old-style self argument lazily allocates raw storage and stores a non-null pointer in the instance.
  3. Converting a later argument can invoke Python—for example, through __index__.
  4. That Python code re-enters a bound method on the same object.
  5. Because the pointer is now non-null, load_value() skips the new guard in type_caster_base.h and dispatches through unconstructed storage.

A virtual call then segfaults. No thread race is required; ordinary same-thread Python re-entry is sufficient.

This is not a regression introduced by the PR, but it remains squarely inside the safety invariant the PR claims to establish. Green CI simply means this path is not tested.

The same design issue also affects:

  • Mixed new-style/old-style overload chains: one old-style overload arms the flag while new-style candidates are tried.
  • Python multiple inheritance: constructing base A enables lazy allocation for an uninitialized base B because the flag belongs to the whole Python instance.
  • Nested initialization, which can placement-construct over an already-live object.
  • Free-threaded concurrent access, where the bit and value pointer are ordinary non-atomic storage.

Recommendations

Before merging:

  1. Add espressolee’s later-argument re-entry regression first.
  2. Replace the instance-wide permission window with authorization tied to the exact old-style constructor candidate, exact value_and_holder, and exact self load.
  3. Ensure every other load rejects reserved-but-unconstructed storage even though its pointer is non-null.
  4. Preserve Henry’s failed-construction cleanup and retry behavior.
  5. Add coverage for mixed old/new overloads and cross-base multiple inheritance. Nested initialization should preferably be rejected.
  6. Correct the stale comment in test_class.py: it says construction_in_progress is set during a pure new-style constructor, but Henry’s commit deliberately leaves it unset.
  7. Refresh the PR’s AI summary after redesigning the mechanism and add a suggested changelog entry.

The cleanest state model is conceptually:

uninitialized → reserved exclusively for old-style self → constructed

Ideally, raw storage would not be published through the instance pointer until the constructor callback succeeds. If it must be published, a separate per-value “reserved but unconstructed” state must be checked before every load.

rwgk added 3 commits August 30, 2026 13:34
Track construction per value-and-holder, grant a one-shot loader-frame permission only to the exact legacy constructor self conversion, and keep its raw storage private until the native callback returns.

Reject reentrant, nested, cross-base, and cross-thread loads while preserving overload fallback, failure cleanup, pickle setstate callbacks, and repeated initialization behavior.
The new detail::instance construction state has cross-DSO semantics that internals-v12 modules do not understand. Isolate the incompatible domains for v3.2.0 and document that future structural or semantic instance changes require another bump.
@rwgk

rwgk commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Conclusion and release sequencing

This fix needs PYBIND11_INTERNALS_VERSION 13. Although the new construction-state flags fit in spare bits and do not change sizeof(detail::instance), they change the protocol that separately compiled extension modules use to interpret a shared instance. Keeping the v12 identifier would allow old and new inline caster code to act on the same object using incompatible rules.

The proposed sequence is therefore:

  1. Land [WIP] Interoperability with other Python binding frameworks #5800 in the 3.1.x line, ideally in 3.1.1.
  2. Hold this PR for 3.2.0, where the internals version can advance from 12 to 13.

For participating extensions that enable #5800's interoperability mechanism, supported conversions can then cross the v12/v13 boundary through the general foreign-type path. The internals boundary continues to provide isolation, while cross-version interoperability has the documented limitations and modest extra cost of that path rather than relying on unsafe shared state.

Why retaining internals v12 would be unsafe

detail::instance is shared across extension modules that use the same internals identifier, but the code that reads it is compiled inline into each module. Existing v12 load_value() code has only two relevant interpretations of the value pointer:

  • A null pointer permits the legacy path to allocate and publish raw storage.
  • A non-null pointer is treated as a live C++ object.

It has no representation for "this exact value slot is currently being constructed; do not load or initialize it."

The new implementation keeps old-style placement-new storage private until the callback has successfully returned, leaving the instance's value pointer null in the meantime. An older v12 module can therefore bypass the new protocol, allocate and publish different raw storage, and hand it to bound C++ code before any object lifetime has begun. Substituting a non-null sentinel does not help: old code would treat the sentinel itself as a valid C++ pointer. Either route can reintroduce the undefined behavior this PR is intended to eliminate.

The bump is consequently required by the changed cross-DSO semantics, not merely by the physical size or offsets of detail::instance.

Alternatives considered

Private storage and null rejection without shared construction state

Keeping old-style storage private and rejecting ordinary null loads is sufficient for the original reproducer and many re-entrant cases when all participating code uses the new headers. It is not a complete replacement for the construction-state flag.

In particular, two threads can begin initializing the same value slot, each placement-construct a private object, and discover the collision only when committing. pybind11 has no generic type-erased operation that can correctly destroy the losing object before its holder has been constructed. More importantly, an already-compiled v12 module would still follow its legacy null-pointer allocation path. Narrowing the fix in this way would therefore weaken the guarantee without avoiding the cross-version incompatibility.

Reusing an existing flag or a pointer sentinel

The existing flags describe independent holder, registration, ownership, aliasing, and layout state. Overloading one of them would make normal initialization and cleanup depend on ambiguous flag combinations. A pointer sentinel is unsafe because every older caster, along with other paths that assume a non-null value pointer denotes a live object, can expose or dereference it.

A synchronized side table

A shared registry keyed by the exact value_and_holder slot could technically represent active construction without adding a field to detail::instance. To cover multiple extension modules, interpreters, Python multiple inheritance, nested calls, and free-threaded execution, it would need a carefully synchronized lifetime and locking protocol. A module-local table would not be sufficient.

This would be a substantial one-off shared-state mechanism to represent one per-slot bit, with additional allocation, hashing, locking, and teardown concerns. It also would not eliminate the internals bump: old v12 code would not consult the table and could still expose unconstructed storage.

Decision

Once the v12/v13 boundary is recognized as necessary, storing the construction state directly in the value slot is the smallest and clearest design. It is naturally shared by modules in the same internals domain, is exact for Python multiple-inheritance layouts, and avoids a separate lookup and lifetime-management subsystem.

The chosen approach is therefore to keep the explicit per-value construction state, bump to internals v13 for 3.2.0, and use #5800 as the general bridge for supported conversions between extensions that deliberately live in different internals domains.

@rwgk

rwgk commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

@oremanj for visibility

@rwgk

rwgk commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

@amjames @henryiii @espressolee Could you please make another pass over this PR with your agents?

@espressolee espressolee left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made another pass over exact head 14e32ae23af529df8d82681c2d3064884b259a3c. I do not see a remaining blocker in this PR.

One process note: the auxiliary peer-agent calls did not return a usable review, so none of the conclusions below rely on peer output. I performed the exact-head code pass and the additional probes directly.

The v13 bump is justified by protocol compatibility, not sizeof(detail::instance). I added a small two-extension cross-DSO probe to test that distinction:

  • fixed producer 4455e3f and a legacy-v12 consumer shared the registered C++ object successfully: 20/20 fresh regular processes and 20/20 fresh 3.14t processes;
  • current-v13 producer and current-v13 consumer retained normal interoperability: 20/20 in both configurations;
  • a current-v13 object passed to that legacy-v12 consumer was rejected with TypeError: 20/20 in both configurations. On 3.14t the GIL remained disabled throughout.

That is the behavior the internals split needs to provide: v12 inline caster code no longer interprets a v13 instance using the old null/non-null protocol, while modules in the new domain still interoperate normally.

I also reran the original later-argument re-entry control. The blocked head 66f8f37 segfaults in 5/5 fresh Debug processes; current head rejects the same path safely in 20/20 regular and 20/20 free-threaded processes. The focused test_class.py suite is 49/49 in regular Debug and 49/49 in 3.14t Debug. GitHub currently reports 76 passing checks and two expected skips.

So the proposed sequence looks sound to me: isolate this construction protocol in internals v13 for 3.2.0, and treat #5800 as the explicit interoperability path for supported cross-domain conversions. I did not independently validate #5800's bridge behavior in this pass, so my approval is scoped to #6157's construction-state fix and the v12/v13 isolation at this exact head.

@henryiii

henryiii commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

I'm nearly sure this does not require a ABI bump, and I asked Fable 5.1 about it too:

🤖 AI text below 🤖

No, I don't think the bump is justified. The argument on #6157 for v13 is that v12 inline caster code, given a v13 instance mid-construction, would still lazily allocate storage. That's true, but it is the pre-existing v12 bug, not a new incompatibility. In every mixed scenario I can construct, behavior is identical to pure v12. The bump doesn't make v12 modules safer; it only stops v12 and v13 modules from sharing types at all, which is a large cost for a bug fix.

The concrete checks:

  • Layout is unchanged. The new simple-layout bit is the 7th bit of an existing bitfield byte, and the nonsimple flag is bit 4 of the existing status byte. Instances come from tp_alloc, which zero-fills, so an instance allocated by a v12 module starts with the bits clear.
  • v12 never clobbers the new bits. All v12 touches to the status byte are |= and &= ~mask on bits 1 and 2. There are no full-byte writes.
  • v13 never depends on v12 honoring the bit. The bit is only set during constructor dispatch, which runs in the module that registered the class. Fully constructed objects look the same to both versions.
  • v13 owns class, v12 loads it while __init__ is re-entered. v12 allocates and publishes garbage storage. That's the old bug, and in pure v12 the same load produced the same garbage. Afterwards v13's commit sees a non-null pointer and hits pybind11_fail, which is a clean error rather than the UB pure v12 gave.
  • v12 owns class, v13 loads it. The constructing bit is never set, so v13 falls through to the null check and raises instead of allocating. Still strictly better than pure v12.

The rest of the comment (thread collision, sentinel pointers, side tables) argues for why the flag should exist in v13, not for why v12 and v13 can't coexist. I'd revert the last commit, keep the change in 3.1.x, and drop the "hold for 3.2.0" sequencing.

One real defect I noticed while reading the mixed case: cleanup_old_style_init_storage in type_caster_base.h:78 calls pybind11_fail from the loader_life_support destructor. If the commit fails, that destructor runs during unwinding and the second throw is std::terminate. It is only reachable via the mixed-version case or a bug, but a destructor should not throw. That is worth a small fix regardless of the ABI decision.

@espressolee

Copy link
Copy Markdown

@henryiii was right about the flaw in my earlier review. My measurements no longer support my objection to the ABI bump, so I am retracting that reasoning. I also found a separate destructor issue while checking it.

Retraction. My approval leaned on a three-arm cross-DSO matrix and said its result was "the behavior the internals split needs to provide." That arm — v13 producer, legacy-v12 consumer, TypeError — only confirms the expected consequence of the bump in that setup: the two modules end up with separate type registries. It cannot distinguish "the bump is needed" from "the bump is not needed." The arm that does bear on the question, unbumped fix producer with a legacy-v12 consumer, shared the object successfully, which supports your reading. And none of the three probes drove the old-style-__init__ re-entry path. I retract that part of my rationale.

The probe I should have run. A producer registers a class with old-style placement-new __init__(T &self, int); while the int is converted, __index__ re-enters and asks a second extension module to load the same still-unconstructed instance.

When the re-entrant load dereferences the object, pure-v12 and unbumped-mixed have the same observed outcome, SIGSEGV — your result, reproduced.

When the re-entrant load only stores the pointer without touching object state:

  • The legacy module's inline load_value publishes lazily allocated storage into the shared value pointer. complete_old_style_init then raises RuntimeError: invalid old-style constructor commit, a catchable Python exception.
  • While that exception is unwinding, ~loader_life_support() hits the storage collision and calls pybind11_fail. I instrumented that branch: std::uncaught_exceptions() == 1 there on every run.
  • Because the destructor is implicitly non-throwing, letting pybind11_fail throw from it terminates the process regardless; in the observed execution it additionally happens while another exception is active. The unbumped mixed build reaches std::terminate where the pure-v12 build returns.
  • In this harness, changing only that destructor path to report through PyErr_WriteUnraisable avoids the termination. The RuntimeError from complete_old_style_init remains catchable and __init__ does not complete, which is the expected high-level failure mode; the destructor-path change would still need review for cleanup and error-reporting semantics.

So this probe does not provide a sound new argument either for or against the bump. In particular, it does not rescue my earlier objection. What it does identify is a separate destructor error-reporting issue: ~loader_life_support() has no exception specification and all its members have non-throwing destructors, so it is implicitly noexcept, and this is the difference between raising an error and killing the host process. The destructor already calls pybind11_fail for the frame-stack invariant on master; the collision check adds another path into the same destructor.

If this diagnosis looks right, I would be happy to open a separate PR against master for the destructor path.

Scope. I reproduced this with clang/libc++ on Mach-O and gcc/libstdc++ on ELF, across the optimization, visibility and GIL configurations recorded in the harness. The probe exercises cross-extension interaction through the shared type registry, not arbitrary source mixing. And the pointer-only case already binds a T& to storage whose object lifetime has not begun, so it contains UB and cannot on its own impose a compatibility requirement.

Harness, pinned trees, build commands and raw results: https://github.com/espressolee/pybind11-6157-reentry-matrix

espressolee pushed a commit to espressolee/pybind11-6157-reentry-matrix that referenced this pull request Sep 2, 2026
Measures whether the old-style placement-new __init__ fix, built without the
internals ABI bump, behaves like a pure v12 build when a second extension
module re-enters during argument conversion and loads the same still
unconstructed instance.

Five configurations across two probe traces, reproduced on Mach-O/clang/libc++
and ELF/gcc/libstdc++ and across optimization, symbol-visibility and GIL
variants. Raw output is in results/; RECEIPT.json carries the inputs, the
toolchains and the claim ceiling.

That ceiling is deliberately narrow. The decisive trace binds a reference to
storage whose object lifetime has not begun, so it measures observable
behaviour rather than defined behaviour, and cannot on its own impose a
compatibility requirement.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
espressolee pushed a commit to espressolee/pybind11-6157-reentry-matrix that referenced this pull request Sep 2, 2026
Measures whether the old-style placement-new __init__ fix, built without the
internals ABI bump, behaves like a pure v12 build when a second extension
module re-enters during argument conversion and loads the same still
unconstructed instance.

Five configurations across two probe traces, reproduced on Mach-O/clang/libc++
and ELF/gcc/libstdc++ and across optimization, symbol-visibility and GIL
variants. Raw output is in results/; RECEIPT.json carries the inputs, the
toolchains and the claim ceiling.

That ceiling is deliberately narrow. The decisive trace binds a reference to
storage whose object lifetime has not begun, so it measures observable
behaviour rather than defined behaviour, and cannot on its own impose a
compatibility requirement.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@rwgk

rwgk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

I ran the below last night, codex gpt-5.6-sol ultra. I won't have time to carefully scrutinize myself until later (weekend maybe).

I just noticed, I mixed up the PR and issue numbers in the /wrk/pr6153abi_question directory name: I accidentally used the issue number, but that's just cosmetic.


Codex transcript

  • Session last activity: 2026-09-01 22:10 PDT
  • Session ID: 01a0605f-3c4a-7da2-9520-ede41bd6128b

User

Could you please act as if you don't know me, i.e. exclude all memory based on other sessions from this context?

With that blank slate:

Could you please fetch a complete view of PR 6157?

Could you please try to resolve a key question:

Is it safe beyond a reasonable doubt that we DO NOT bump the PYBIND11_INTERNALS_VERSION?

On a chat with Henry I argued:

Henry Schreiner  [8:35 PM]
I responded. I still do not believe that it needs a version bump (and Fable 5.1 agrees)

Ralf Grosse-Kunstleve  [9:21 PM]
Maybe you're right, but only under certain conditions. It's not easy to be sure. It needs a very careful look, or risk taking. — I want to say the bump combined with merging #5800 is definitely safer. But then again, we don't have #5800 merged yet.

I was thinking of pressing codex or Fable (independently from the context you have already) hard to construct an example that breaks without the v13 bump. If no agent can do it, I'd think it's safe enough.


Could you please use the resources on this machine to try hard to construct an example?

Please do all work under: /wrk/pr6153abi_question
Please create the directory.

Feel free to do anything you need under that directory, but do not modify any files outside that directory (apart from /tmp and other standard locations that you usually have write access to, e.g. your CODEX_HOME).

Also feel free to use gh for read-only external access.

I'm guessing that free-threaded Python is more likely to create situations that may result in undefined behavior without an internals version bump. Please use these existing from-source installations, so you can experiment with matching default and freethreaded Python builds:

/wrk/cpython_installs/v3.14.4_23116f998f6_default
/wrk/cpython_installs/v3.14.4_23116f998f6_freethreaded

Feel free to create venvs under /wrk/pr6153abi_question as you see fit.

Could you please do a pre-flight analysis of my request. Are there additional things I should define before you start?

Codex

Yes. I’ll treat this as a blank-slate investigation, using only this conversation, PR data, repository history, and experiments performed for this task.

Nothing essential is missing. I can proceed with these assumptions:

  • All writes, builds, caches, venvs, logs, and reports will live under /wrk/pr6153abi_question or /tmp.
  • /wrk/forked/pybind11 and the two CPython installations will be treated as read-only.
  • GitHub access will be read-only.
  • “Complete view” includes PR 6157’s diff, commits, reviews, discussion, checks, related issue/PR context (especially 6153 and 5800), and the unpushed local merged state.
  • “Safe beyond a reasonable doubt” means mixed pybind11 extension modules—one built before the change and one after it—can safely share the same internals capsule in either import order. I’ll look for layout, ownership, lifetime, synchronization, and semantic incompatibilities, not merely compilation failures.
  • I may create instrumented header variants inside the work directory, including a synthetic internals-version bump as a control.

My investigation would combine:

  1. Reconstructing the exact relevant revisions: base, PR tip, current master, and the local amjames/bugfix-6153 state.
  2. Performing a static ABI analysis of every changed path that can enter or interpret shared internals.
  3. Building adversarial pairs of extension modules against old/new headers and loading them into one process in both orders.
  4. Passing registered types and affected objects across the module boundary, testing destruction/finalization, repeated import cycles, subinterpreters where relevant, and concurrent operations.
  5. Running the matrix on both supplied CPython 3.14.4 builds, with extra stress on free-threaded Python.
  6. Using available debug and sanitizer configurations where they produce meaningful evidence.
  7. Comparing behavior with artificially separated internals as a control.
  8. Producing a reproducible report and preserving all test sources, commands, logs, and binaries under the requested directory.

One methodological caveat: failure to find a reproducer cannot itself prove safety. I’ll therefore only recommend no bump if the static compatibility argument closes the plausible failure modes and the experiments corroborate it. If I find an unresolved mixed-version interpretation or lifetime risk—even without a reliable crash—I’ll call that out rather than declaring it safe.

No further definition is required before starting. An optional time limit would affect how long I run concurrency stress tests, but absent one I’ll use a practical, thorough stopping point and document the coverage.

User

Please go ahead.

Codex

I’m starting with a contained workspace and an evidence log. I’ll first reconstruct the PR and revision topology, then split the static ABI audit and mixed-header reproducer work so the conclusions can be independently cross-checked.

Codex

The local topology already reveals an important fact: the unpushed branch contains a final commit titled fix: bump internals version to 13; the preceding commit is the merged/fixed PR state without that bump. I’m treating those two adjacent states as the main A/B comparison while I verify the public PR head and discussion independently.

Codex

The static threat model now has a concrete mixed-version failure path: during a new-header legacy constructor, storage is deliberately private and the new instance bit says “constructing.” An old-v12 consumer understands neither rule; when asked to cast that same Python object, it can allocate and publish a second raw storage block. The new code then encounters two competing storages. I’m turning that exact path into a deterministic two-extension test, with v13 separation as the control.

Codex

The first full default-CPython run is decisive, and it fails in two independent ways without the bump:

  • The original __new__ bypass remains exploitable through an old-v12 consumer: it publishes raw 0xA5-filled storage; reading returns 0xA5A5A5A5A5A5A5A5, and virtual dispatch segfaults.
  • Re-entering an old-v12 consumer during the PR’s protected constructor aborts in both import orders with old-style constructor storage collision.

With v13, the old consumer has a separate registry and rejects the object with a normal TypeError. I’m now repeating the same controls on the supplied free-threaded build and checking the reverse-direction compatibility surface.

Codex

The free-threaded and reverse-direction matrices match the default build exactly. An independent 60-process harness also completed: 44 safe/control exits, 8 deterministic SIGABRTs, and 8 deterministic SIGSEGVs. UBSan additionally identifies the old-consumer access as “does not point to an object of type Victim / invalid vptr,” so this is confirmed undefined behavior rather than merely an inconvenient exception path. I’m consolidating the PR record, counterarguments, and fully reproducible evidence now.

Codex

Conclusion

No. It is demonstrably unsafe to omit the bump. Keep PYBIND11_INTERNALS_VERSION == 13.

I constructed concrete mixed-v12 failures:

  • An old-v12 caster can publish raw storage into an uninitialized object created by the patched module. Patched code then trusts that pointer; UBSan reports an invalid vptr, and virtual dispatch receives SIGSEGV.
  • During the patched legacy-constructor protocol, new code privately reserves storage A while an old caster publishes storage B. Completion consistently terminates with SIGABRT and old-style constructor storage collision.
  • The reverse old-owner/new-consumer direction also exposes unconstructed storage.
  • These reproduced on both supplied CPython builds, in both import orders, including cross-thread execution.

The independent 60-process matrix produced:

  • 44 clean/control exits
  • 8 deterministic SIGABRTs
  • 8 deterministic SIGSEGVs

The v13 controls separate the registries and safely reject these casts with TypeError.

Henry’s layout observations are correct: sizeof(detail::instance) remains 56 bytes on default CPython and 72 on free-threaded CPython. But this is a semantic state-machine incompatibility, not a size/offset incompatibility. Old inline code cannot understand the new construction bit or private-storage protocol.

The argument that old-v12 was already unsafe does not resolve this: without the bump, an old caster can poison a new module’s object so that patched code itself accepts invalid storage. The mixed constructor also creates a two-storage collision absent from the pure-v12 protocol.

At query time, PR 6157 already had public head 14e32ae2, including the v13 bump; the initial local-only premise had become stale. Exact-head CI showed 76 successes and two expected skips. PR 5800 is not a safety substitute: it remains WIP/conflicting and cannot retroactively bridge arbitrary deployed v12 binaries.

There is also a separate real defect: cleanup can throw from loader_life_support destruction during unwinding, causing std::terminate. That should be fixed, but it does not eliminate the poisoning or invalid-object paths.

Artifacts:

  • Complete investigation report — pr6153abi_question/FINAL_REPORT.md
  • Independent reproducer report — pr6153abi_question/reproducer_agent/artifacts/REPORT.md
  • 60-case matrix — pr6153abi_question/reproducer_agent/artifacts/logs/matrix.json
  • Reproducer sources — pr6153abi_question/reproducer_agent/artifacts/src
  • UBSan results — pr6153abi_question/results-default-sanitized-uninitialized.json

The original /wrk/forked/pybind11 repository remains clean and unchanged.


pr6153abi_question_2026-09-01+230258.zip

@rwgk

rwgk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

@espressolee

If this diagnosis looks right, I would be happy to open a separate PR against master for the destructor path.

It looks like all our agents found that issue. It'd be great if you could send a separate PR.

@henryiii

henryiii commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

That a previous version can poison the new version with the old bug if we both are v12 is not a valid argument for bumping to v13. It's taking currently buggy behavior and saying that remains buggy if one side doesn't update. Bumping the ABI has a huge cost; it forces everything to be recompiled and support for old versions to be dropped. Keeping it v12 allows both sides to update on their own time, which speeds up the update process dramatically. Things like PyTorch cannot update to a new pybind11 ABI unless they make a major bump themselves.

If this avoids a new bug, that could be reason to bump, but the examples above are things that are currently buggy. Remember ABI bumps only add forced breakages! It should be used when the two sides are incompatible.

(after writing that, I asked Fable 5.1 to see what it thinks, response below)

🤖 AI text below 🤖

Your position holds. Every reproducer Codex found starts with a v12 module executing its known lazy-allocation bug on an unconstructed object. None of them is reachable from code that works today without a bump.

What the new comments actually establish:

  • espressolee retracted the ABI argument. Their re-run shows pure v12 and unbumped mixed give the same segfault when the re-entrant load dereferences the object. The only difference is in the pointer-only case, where mixed hits the destructor throw and terminates instead of continuing with UB. They correctly note the pointer-only case is itself UB and cannot impose a compatibility requirement.
  • Codex's "poisoning" case is the __new__ bypass object passed first to a v12 consumer. The v12 consumer allocates garbage and dereferences it, which is the original bug and identical to pure v12. That v13 code then also sees the garbage adds nothing new, and the bump does not fix it for objects owned by v12 modules either.
  • Codex's "two-storage collision" is the later-argument re-entry through a v12 module during an old-style __init__. Pure v12 gives UB there. Mixed gives UB in the v12 module followed by a hard abort from the throwing destructor. That abort is the separate destructor bug, not an ABI incompatibility.
  • The bump does not make anything safer. It only stops v12 and v13 modules from sharing types at all. Codex's "controls" show a TypeError, which is the interop breaking, not the bug being caught.

One honest caveat: there is one pattern where unbumped mixing is worse than pure v12. An old-style __init__ that, after its placement-new, hands self to a function in a v12 module. In pure v12 that works. In pure v13 it raises ValueError because storage stays private until commit, which is the intended behavior change of this PR. In mixed mode the v12 module allocates garbage and the commit aborts. So the PR breaks that pattern with or without the bump, and it is already gated behind a debug-only FutureWarning at pybind11.h:748.

Recommended reply: keep v12, and fix the destructor so it never throws. On collision it should free its private storage, clear the pointer, and report via PyErr_WriteUnraisable. With that, the mixed collision becomes a catchable RuntimeError from complete_old_style_init rather than std::terminate, which is strictly better than pure v12's UB. espressolee offered a separate PR for that.

@rwgk

rwgk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

a previous version can poison the new version with the old bug if we both are v12

It means that the bug isn't fixed in general, only maybe, for a given application, until we finally bump the ABI.

But if we are clear about that, I agree it's better than not doing anything at all.

This seems fine:

  • Merge this PR without the bump, documenting clearly that we're not reliably fixing 6153.
  • Resolve the throwing-destructor path before making a new 3.1 release.
  • Finalize PR 5800 and merge, then make a 3.1 release, still without bumping the ABI.
  • Then bump the ABI, and declare that 6153 is fixed reliably in 3.2+.

@henryiii

henryiii commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Yes, though we need a reason to bump the ABI for 3.2, not just to "fix" this. I'm pretty sure we will have one, but bumping the ABI is not to force people to upgrade.

It means that the bug isn't fixed in general, only maybe, for a given application, until we finally bump the ABI.

It means to fix the bug, you need to upgrade pybind11. Not bumping the ABI makes it easier to upgrade pybind11. For example, if PyTorch wants to fix it, they upgrade pybind11 (which they can do trivially if there's no ABI bump), then any extension can work correctly by upgrading pybind11 (which they can do, because there's no ABI bump). It's just like any other bugfix.

ABI bumps are not for fixing bugs. They are for incompatible changes.

@rwgk

rwgk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

but bumping the ABI is not to force people to upgrade.

Agreed. That's not what I meant to imply. But someone who prioritizes safety will need the ABI bump.

I'm working on reverting the last commit.

@rwgk

rwgk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

I'm working on reverting the last commit.

codex recommends (more-or-less) folding the throwing-destructor fix into this PR.

@espressolee I'll tell it to do that now; I have the context already, it's just one prompt.

@henryiii

henryiii commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Folding it in makes sense to me.

@rwgk
rwgk requested a review from henryiii as a code owner September 2, 2026 20:59
@rwgk

rwgk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

@henryiii @espressolee — please see the codex gpt-5.6-sol ultra generated explanation below.

One thing that crossed my mind but I haven't look into yet: what is the runtime impact of the "transactional rollback"?


I folded the mixed-v12 storage-collision recovery into this PR in commit bda1151.

Thank you both for identifying the destructor problem. The pointer-only reproducer reaches a specific sequence:

  1. Updated code privately reserves storage A for an old-style constructor.
  2. A stale v12 caster re-enters, sees the public value pointer as null, and publishes competing storage B.
  3. complete_old_style_init() detects B while trying to commit A and throws.
  4. The old cleanup path then also threw from the implicitly noexcept loader_life_support destructor, terminating the process.

The new behavior is transactional rollback:

  • If argument conversion fails before the constructor callback runs, cleanup removes B and frees both raw allocations. Normal conversion failure remains the visible outcome, rather than a cleanup error or process termination, and another overload or a later __init__ attempt starts with an empty slot.
  • If the callback has already placement-constructed A, complete_old_style_init() removes B, temporarily initializes the real holder for A, and deallocates A through that holder. This runs A's C++ destructor, preserves the registered holder/delete behavior, and then raises a catchable RuntimeError for the collision. The Python object is again empty and can be initialized later.
  • The B == A case is handled as one allocation, avoiding a double free.
  • Loader cleanup is now non-throwing. Exceptions from a registered cleanup callback are reported with PyErr_WriteUnraisable, and the slot is abandoned rather than attempting the same potentially partial destruction again.

Raw private storage is freed through the type_info::dealloc callback from the DSO that registered the type, using a synthetic holder-free value_and_holder. That preserves class-specific and aligned operator new/delete pairing without publishing A through the real Python instance.

The regression test freezes the relevant lazy-publication fragment from the pre-PR v12 caster (f90c430c) in the cross-module test extension. It covers conversion failure, collision after successful placement construction, retryability, exact allocation/construction/destruction counts, the default holder, and py::smart_holder. It also verifies that the supported paths produce no unraisable cleanup errors.

Local validation:

  • test_class.py: 50/50 passed on CPython 3.14.4.
  • test_class.py: 50/50 passed on free-threaded CPython 3.14.4 with the GIL disabled.
  • A separately compiled genuine-v12 consumer paired with the updated producer recovered cleanly in 20/20 fresh regular processes and 20/20 fresh no-GIL processes. The old result was deterministic SIGABRT; the new result is the catchable collision RuntimeError, followed by the expected uninitialized-object ValueError if the object is used without retrying __init__.
  • The same genuine-v12 probe passed under ASan/UBSan.
  • C++11 warnings-as-errors compilation and prek -a --quiet passed.

This deliberately does not claim to make arbitrary mixed-v12 execution safe. Stale v12 code can still dereference B before updated code regains control, or retain B after returning; rollback cannot repair an escaped pointer. That is the pre-existing v12 bug and should remain explicit in the PR description / release note. This commit removes the new hard-termination behavior and leaves the updated side in a defined, retryable state when the stale call only publishes or records the pointer.

@rwgk

rwgk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

@rwgk asked codex:

What is the runtime impact of the "transactional rollback"?

codex

Short answer: the normal-path runtime impact should be negligible. “Transactional” is probably too grand a word—it is slot-level exceptional cleanup, not an always-on transaction mechanism.

  • Ordinary calls and successful constructors gain no allocation, copy, log, or additional lock.
  • Successful old-style construction performs the same two checks as before; they are merely split so the collision branch can be handled.
  • Failed construction without a collision adds a small stack-initialized synthetic instance, but no additional heap allocation.
  • Only an actual collision pays the full cost: deallocate B, initialize/register A’s holder, deregister/deallocate A, and translate a RuntimeError. User-defined destruction may dominate that cost. Previously this path terminated, so there is no meaningful throughput regression to compare against.

A paired microbenchmark found no stable performance signal—the result changed sign between default and free-threaded Python. Minimal extension text size grew approximately 0.5–1.1% (about 1–2 KiB), which is the clearest measurable cost.

The more important impact is semantic:

  • A’s constructor has already run, so arbitrary constructor side effects are not rolled back.
  • Holder/destructor side effects happen synchronously during cleanup.
  • Escaped A or B pointers can become dangling.
  • “A’s destructor runs” is true for the tested owning default and smart_holder cases, but not universally—for example, shared/external ownership may defer destruction, and nodelete deliberately suppresses it.

So I would call it “transactional rollback of pybind11’s value slot, registration, and holder state,” not a general C++ transaction. That qualification would be worth adding beneath your posted explanation.

@rwgk

rwgk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

@rwgk asked codex:

  I see:

  --------------------------------------------------------------------------------------------------------------------------------------
  File                                                                                             blank        comment           code
  --------------------------------------------------------------------------------------------------------------------------------------
  include/pybind11/detail/type_caster_base.h
   same                                                                                                0            310           1223
   modified                                                                                            0              1              6
   added                                                                                              25             38            233
   removed                                                                                             0              0              9

  If someone asked "why so many changes here", what would be the explanation? (In the back of my mind: it almost seems there is a
  completely overlooked missing feature, rather than a bug fix.)

codex

Your instinct is directionally right: this is a bug fix at the public-contract level, but it exposed a missing internal lifecycle mechanism.

Previously, load_value() at include/pybind11/detail/type_caster_base.h:1408 effectively treated these distinct states alike:

  1. A Python wrapper exists, but no C++ object was constructed.
  2. Raw storage exists for a legacy placement-new constructor.
  3. A valid C++ object exists.

For a __new__-only object, it allocated raw memory and returned it as though it contained a live C++ object—hence the undefined behavior.

The obvious small fix, “reject a null value pointer,” would break legacy __init__ and __setstate__. Preserving those requires the added machinery:

  • Track construction for the exact value_and_holder, including multiple inheritance.
  • Authorize exactly one legacy constructor self conversion.
  • Keep raw storage private so reentrant or concurrent code cannot observe it.
  • Publish it only after the C++ callable succeeds.
  • Roll it back on conversion failure, exceptions, or overload fallback.
  • Use the correct custom/aligned allocator, deallocator, and holder behavior.
  • Synchronize the state for free-threaded Python.
  • Because we retain v12, detect and recover from competing storage published by stale inline caster code from another extension module.

Most of the header growth is that transaction:

  • About 175 code lines: legacy-constructor reservation, commit, and rollback.
  • About 45: per-value construction scope.
  • About 18: enforcement in load_value().
  • The v12 collision recovery alone added roughly 63 non-comment code lines.

So I would explain it this way:

#6153 has a small symptom, but it revealed that pybind11 did not explicitly model the interval where a Python wrapper exists but its C++ value is still under construction. The old lazy-allocation shortcut was also how legacy placement-new constructors worked, so a null check alone would break compatibility. Most of this patch implements the missing constructor transaction—scoped authorization, private storage, synchronization, commit, and rollback—and makes it safe across exceptions, overloads, reentrancy, multiple inheritance, free threading, and mixed v12 modules.

I would call it “missing internal machinery exposed by a bug,” rather than a missing user-facing feature. A substantially smaller change would require dropping legacy constructor behavior, bumping the internals version, or knowingly leaving some cases unsafe. That said, the explanation justifies why the fix cannot be a trivial guard; it does not by itself prove every line is minimal.

@rwgk

rwgk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

@henryiii @espressolee I just updated the PR description. This PR is ready for another review.

@espressolee espressolee left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-ran the paired re-entry matrix at head 23f2d0a, with the 09-02 unbumped head 4455e3f as a same-run reference. macOS arm64, Apple clang 21, -O1 -fvisibility=hidden, CPython 3.14.6 and 3.14.0rc1t, 20 fresh processes per cell; nm -gU shows no module exports load_value, which rules out exported-symbol interposition.

trace producer + consumer 4455e3f 23f2d0a
pointer-only patched + v12 std::terminate 20/20 __init__ raises RuntimeError; instance left unconstructed, get()ValueError 20/20
pointer-only v12 + v12 completes 20/20 completes 20/20
pointer-only patched + patched ValueError rejection 20/20 same 20/20
deref patched + v12 SIGSEGV 20/20 SIGSEGV 20/20
deref v12 + v12 SIGSEGV 20/20 SIGSEGV 20/20
both v12 producer + 23f2d0a consumer identical to pure v12

Same on both interpreters; stderr stayed empty in every 23f2d0a cell, so I observed no unraisable cleanup report.

That matches the diff: cleanup_old_style_init_storage() is noexcept with its deallocation calls inside try/catch, so that cleanup path no longer propagates those exceptions, and the remaining collision pybind11_fail sits in complete_old_style_init(), the ordinary catchable path. One non-blocking reading: in that collision branch, if deallocate_instance_value(v_h) throws on the stale storage ("could not deregister"), old_style_init_storage has already been cleared, so the placement-constructed private value is leaked undestroyed — an internal-inconsistency path, fine to leave.

Approving: the pointer-only mixed case no longer takes the process down, no other tested arm changed outcome, and the code reads consistently with the measurements. Not exercised here: the rollback with smart_holder or custom allocators; for those I am relying on the tests in bda1151. Runner and raw results: https://github.com/espressolee/pybind11-6157-reentry-matrix/tree/main/results-23f2d0a7

The loader frame already identifies the constructor candidate, so the
one-shot `self` permission only needs a frame match and a claimed flag.
This removes both argument guard classes, the changes to cast.h, and the
per-call TLS lookups they added.

Also:
- Hoist deallocate_instance_value to a detail free function and use it
  from instance_construction_scope.
- Take the dispatcher's constructor lock before the construction scope and
  drop the nested critical sections it made redundant.
- Keep the non-constructor path inline: the loader destructor checks for
  storage before the out-of-line cleanup, and the construction scope
  defaults to not started.
- Commit old-style storage once in cpp_function::initialize, gated on
  is_constructor.
- Share one __index__ probe across the reentrancy tests and turn the
  subprocess script into a plain function.

Assisted-by: ClaudeCode:claude-fable-5-1
@henryiii

henryiii commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

This is getting really complex (1K new lines!), with a lot of complexity for corner cases in something that is already broken in released pybind11. If we didn't have coding agents spewing out code, this would not have been a 1K fix. I am fine with a more practical approach over a completely all-edge-cases covered one.

I ran /simplify, which cut a bit. There are also a few suggested simplifications that are not purely cosmetic (items 1-3). I'd be fine with any or all of those three.

🤖 AI text below 🤖

Done. All fixes are applied as uncommitted changes on bugfix-6153, and the full suite passes on 3.13 (1192 passed) plus the PR's tests on free-threaded 3.14t (82 passed). Net effect: 181 insertions, 318 deletions against the PR.

Fixed

  • Dropped both permission guard classes and the allowed flag. The frame already identifies the candidate, so try_reserve_old_style_init_storage now checks frame match plus one-shot claimed. This removes the per-call TLS lookups in argument_loader::load_one and call_impl, and cast.h no longer changes at all.
  • Hoisted deallocate_instance_value to a detail free function. It replaces the duplicated deregister-then-dealloc sequence in the construction scope destructor.
  • Removed the nested critical sections in cleanup_old_style_init_storage, complete_old_style_init, and instance_construction_scope. The dispatcher's constructor_lock now comes before the scope and covers all of them, which also avoids the 3.13t slow path for same-object nesting.
  • Kept the hot path inline. The loader destructor checks storage before calling the now PYBIND11_NOINLINE cleanup. The construction scope has a started_ = false default and out-of-line start/finish.
  • cpp_function::initialize calls complete_old_style_init once, gated on is_constructor, instead of unconditionally in both branches.
  • Dispatcher names the old-style predicate once as a local lambda and restores the original "can't support repeated __init__ in C++" comment.
  • try_reserve returns the pointer instead of bool plus out-param. Cleanup reports errors directly from the catch blocks instead of via deferred flags.
  • Tests: a shared _LoadOnIndex probe replaces five near-identical __index__ classes. The 110-line f-string subprocess script is now a plain, linted function with pytest.raises and a three-case loop. The legacy caster emulation drops the aligned-new ladder that no test type reaches.

Skipped

  1. Design-level finding from the altitude review: private storage is what creates the mixed-v12 "collision" that the rollback machinery then has to handle. If the authorized self load allocated straight into value_ptr() (still gated by the value_constructing flag), roughly 100 lines of rollback, the fake-instance dealloc helper, the cross-module emulation, and the subprocess test would go away. That reverses a deliberate PR decision discussed in the PR threads, so it is a question for the author, not a cleanup.
  2. Unconditional critical section in load_value on free-threaded builds adds a lock per bound-class argument on every call. A check-then-lock ordering would fix it but relaxes the PR's stated memory-model guarantee. Also for the author.
  3. The private_storage_is_published branch looks unreachable, but removing it risks a double free if I missed a path, so it stays.
  4. The C++ test structs and the two collision binding blocks could share a template, but the gain is marginal.

@espressolee espressolee left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-ran the same matrix at 89a5f72e: every cell identical to 23f2d0a7, on CPython 3.14.6 and 3.14.0rc1t, 20 fresh processes each. The pointer-only mixed-v12 case still raises a catchable RuntimeError from __init__ and leaves the instance unconstructed, stderr empty; the dereferencing cases still produce SIGSEGV, as pure v12 does; a v12 producer with an 89a5f72e consumer still gives the same outcomes as pure v12. 4455e3f is carried in the same runs and still terminates, so the rig can still show that failure when it is there.

So the refactor is behaviour-preserving on the two traces this harness covers. Still one toolchain and one OS, and it does not exercise smart_holder, custom allocators, or multiple inheritance.

Results: https://github.com/espressolee/pybind11-6157-reentry-matrix/tree/main/results-89a5f72e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants